Added support for importantForAccessibility for some components #31296

grgr-dkrk · 2021-04-04T06:53:27Z

Summary

This PR supports importantForAccessibility on some components (Picker, Button, ImageBackground).

These components convert importantForAccessibility="no" to importantForAccessibility="no-hide-descendants".
This is to prevent focusing on the offspring elements of the component.

Picker is a deprecated component, so I will also work with @react-native-picker/picker if this PR is accepted.

Changelog

[Android] [Fixed] - Added support for importantForAccessibility for some components(Button, Picker, and ImageBackground)

Test Plan

add snapshot.
capture the GIF

analysis-bot · 2021-04-04T07:18:02Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 36f3bf2

kacieb · 2021-04-16T22:24:42Z

Libraries/Components/Picker/PickerAndroid.android.js

@@ -110,6 +111,12 @@ function PickerAndroid(props: Props): React.Node {
    ],
  );

+  // If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the Picker.Item should not be focused.


Hey! I just wanted to ask why this is needed? What happens if someone specifies "no" without this code?

Thanks. If set to no, the Picker will not focus, but it will focus on the TextView inside that.

Good catch @grgr-dkrk !

@kacieb , this is due to the fact that while some components like <Button> seem like atomic units, they actually end up rendering a hierarchy of components for example a wrapper <View> with a <Text> inside them.

By allowing those descendants to become focusable, it sort of breaks the model of these being a single component to consider, so I think forcing no-hide-descendants makes sense here.

The other option is to always render any descendants of these elements with importantForAccessibility="no", so that they can never be focusable on their own. This would have the same end result, but may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label (which is what Talkback does with unfocusable text elements by default).

kacieb · 2021-04-16T22:25:15Z

Thanks for working on this! cc @blavalla to check the Android behavior.

blavalla

Overall this approach looks good to me. I am not sure if it makes sense on ImageBackground though. Waiting for @kacieb to clarify the common usage of that component and whether this pattern fits with it or not.

blavalla · 2021-04-19T21:23:11Z

Libraries/Image/ImageBackground.js

@@ -57,9 +57,16 @@ class ImageBackground extends React.Component<$FlowFixMeProps> {
  render(): React.Node {
    const {children, style, imageStyle, imageRef, ...props} = this.props;

+    // If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the text inside should not be focused.


@kacieb , do you think this same logic makes sense for "ImageBackground"? I am not sure where/how this component is commonly used, but if people tend to consider it a wrapper element that does not impact it's children, I don't think this makes sense necessarily.

Basically, this would cause all children passed into this background to not be focusable even if you only wanted the wrapper itself to not be focusable. Since you are passing these children in yourself (unlike button or picker), this doesn't seem to make as much sense to me.

Based on this RNTester example I found, one possible use case of ImageBackground is to nest content:

So it seems to me like we want to allow children to be able to be focusable, still, or at least allow the React Native user to choose which they'd prefer.

cc @lunaleaps in case you have any additional context on Image that I'm missing.

Sorry had missed this comment. Agreed, we should offer the flexibility for focussed children elements nested in an ImageBackground. All my context comes from is the docs: https://reactnative.dev/docs/next/imagebackground

blavalla · 2021-04-23T23:46:41Z

Libraries/Image/ImageBackground.js

@@ -57,16 +57,9 @@ class ImageBackground extends React.Component<$FlowFixMeProps> {
  render(): React.Node {
    const {children, style, imageStyle, imageRef, ...props} = this.props;

-    // If `no` is specified for `importantForAccessibility`, it will be changed to `no-hide-descendants` because the text inside should not be focused.


Won't importantForAccessibility no longer work at all on ImageBackgrounds now?

I think we should still have this property, it just should work like normal where no = no and no_hide_descendants = no_hide_descendants.

If this is magically being inherited from somewhere else and it does work still, feel free to ignore my comment :)

I seem to have misunderstood.
ImageBackgrounds' importantForAccessibility is given to the Image inside, not the Wrapper itself, and it doesn't seem to work because of that.
I will fix this. Thanks！

lunaleaps · 2021-05-03T17:31:20Z

@grgr-dkrk Is this ready for review again? If not, can you request for review when it is? Or let me know if you have already (sorry a little disoriented in github!) -- just want it to be clear when you're waiting on review!

grgr-dkrk · 2021-05-04T01:27:51Z

@lunaleaps It slipped my mind. Thanks!

kacieb

Small comment about the Picker component snapshot tests, but otherwise looks good to me!

kacieb · 2021-05-06T16:46:24Z

Libraries/Components/Picker/__tests__/__snapshots__/Picker-test.js.snap

@@ -1,5 +1,201 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`<Picker /> should be set importantForAccessibility={no-hide-descendants} when importantForAccessibility={no}: should deep render when mocked (please verify output manually) 1`] = `
+<View>
+  <RCTPicker


I don't see anything about the importantForAccessibility prop here in the snapshot tests, which is unexpected. Does this snapshot need to be updated? Or perhaps we should allow this to be mocked to ensure importantForAccessibility is being set correctly? It seems like right now this test will not fail if the internal behavior is changed.

@kacieb
I was taking a snapshot of iOS, but since this feature is for Android only, I decided to add the test only to the Android components. Thanks!

analysis-bot · 2021-06-25T12:13:17Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,716,028	+0
android	hermes	armeabi-v7a	7,244,255	+0
android	hermes	x86	8,135,345	+0
android	hermes	x86_64	8,100,900	+0
android	jsc	arm64-v8a	9,635,310	+0
android	jsc	armeabi-v7a	8,551,062	+0
android	jsc	x86	9,648,919	+0
android	jsc	x86_64	10,258,149	+0

Base commit: 36f3bf2

fabOnReact · 2021-12-20T05:36:37Z

I will invest time reviewing this pull request and I will add my comments in this pull request. Thanks a lot.

fabOnReact · 2022-07-22T08:54:07Z

testing ImageBackground importantForAccessiblity

365b3d3

2022-07-22.16-49-37.mp4

…34245) Summary: Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], fixes the following issues: 1) ImportantForAccessibility="no" does not work on Text, Button 2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground. Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants. Note: [Button component expected behavior for prop importantForAccessibility][4] >Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here. >The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default). Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5] fixes #30850 related #33690 ## Changelog [Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground Pull Request resolved: #34245 Test Plan: 1) testing ImageBackground importantForAccessiblity ([link to test][1]) 2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5]) 3) testing ImageBackground importantForAccessiblity ([link to test][6]) 4) Button with importantForAccessibility=no ([link to test][7]) [1]: #31296 (comment) "" [2]: https://github.com/grgr-dkrk "grgr-dkrk" [3]: #31296 "#31296" [4]: #31296 (comment) "expected behaviour with prop importantForAccessibility in Button component" [5]: #30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images" [6]: #34245 (comment) "testing ImageBackground importantForAccessiblity" [7]: #34245 (comment) "Button with importantForAccessibility=no" Reviewed By: cipolleschi Differential Revision: D38121992 Pulled By: dmitryrykun fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821

…acebook#34245) Summary: Previously published by [grgr-dkrk][2] as [https://github.com/facebook/react-native/issues/31296][3], fixes the following issues: 1) ImportantForAccessibility="no" does not work on Text, Button 2) ImportantForAccessibility="no-hide-descendants" does not work on Text, Button, or ImageBackground. Note: On ImageBackground, focus is prevented on the imageBackground node itself, but focus is not prevented on its descendants. Note: [Button component expected behavior for prop importantForAccessibility][4] >Some components like Button seem like atomic units, but they end up rendering a hierarchy of components for example a wrapper View with a Text inside them. Allowing those descendants to become focusable, breaks the model of these being a single component to consider and forcing no-hide-descendants makes sense here. >The other option is always to render any descendants of these elements with importantForAccessibility="no", so they can never be focusable on their own. This would have the same result, **BUT may potentially cause issues when the descendant content is supposed to automatically get moved up to the focusable ancestor to act as a label** (which is what Talkback does with unfocusable text elements by default). Note: [importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images][5] fixes facebook#30850 related facebook#33690 ## Changelog [Android] [Fixed] - adding importantForAccessibility for Text, Button, ImageBackground Pull Request resolved: facebook#34245 Test Plan: 1) testing ImageBackground importantForAccessiblity ([link to test][1]) 2) importantForAccessibility="no" does not allow screenreader focus on nested Text Components with accessibilityRole="link" or inline Images ([link to test][5]) 3) testing ImageBackground importantForAccessiblity ([link to test][6]) 4) Button with importantForAccessibility=no ([link to test][7]) [1]: facebook#31296 (comment) "" [2]: https://github.com/grgr-dkrk "grgr-dkrk" [3]: facebook#31296 "facebook#31296" [4]: facebook#31296 (comment) "expected behaviour with prop importantForAccessibility in Button component" [5]: facebook#30850 (comment) "importantForAccessibility=no does not allow screenreader focus on nested Text Components with accessibilityRole=link or inline Images" [6]: facebook#34245 (comment) "testing ImageBackground importantForAccessiblity" [7]: facebook#34245 (comment) "Button with importantForAccessibility=no" Reviewed By: cipolleschi Differential Revision: D38121992 Pulled By: dmitryrykun fbshipit-source-id: 368b4dcb47d7940274820aa2e39ed5e2ca068821

fabOnReact · 2023-01-11T20:34:54Z

These changes landed in the main branch with #34245. For this reason I'm moving the PR to done. Thanks a lot.

github-actions · 2023-01-11T21:05:49Z

	Warnings
⚠️	Libraries/Components/Button.js#L14 - Libraries/Components/Button.js line 14 – Requires should be sorted alphabetically (lint/sort-imports)
⚠️	Libraries/Components/tests/Button-test.js#L8 - Libraries/Components/tests/Button-test.js line 8 – Requires should be sorted alphabetically (lint/sort-imports)
⚠️	Libraries/Image/ImageBackground.js#L13 - Libraries/Image/ImageBackground.js line 13 – Requires should be sorted alphabetically (lint/sort-imports)
⚠️	Libraries/Image/tests/ImageBackground-test.js#L14 - Libraries/Image/tests/ImageBackground-test.js line 14 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against b8747ee

github-actions · 2023-12-21T05:50:56Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2023-12-29T05:01:08Z

This PR was closed because it has been stalled for 7 days with no activity.

fix: ImportantForAccessibility

c4a4685

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 4, 2021

fix: change types of importantForAccessibility

94b44c1

amarlette requested a review from kacieb April 5, 2021 21:50

amarlette added the Accessibility label Apr 5, 2021

grgr-dkrk mentioned this pull request Apr 9, 2021

[Android]Blocking elements from being focused react-native-picker/picker#241

Open

kacieb reviewed Apr 16, 2021

View reviewed changes

kacieb requested a review from blavalla April 16, 2021 22:25

kacieb self-assigned this Apr 16, 2021

blavalla reviewed Apr 19, 2021

View reviewed changes

chore: remove _importantForAccessibility on ImageBackground

a70b59e

blavalla reviewed Apr 23, 2021

View reviewed changes

fix: Fixed the behavior of importantForAccessibility in imagebackground

9109561

grgr-dkrk requested review from blavalla and kacieb May 4, 2021 01:00

kacieb suggested changes May 6, 2021

View reviewed changes

grgr-dkrk added 2 commits May 10, 2021 23:32

chore: add test for picker.android.js

21f3a6b

chore: add comment on Picker-test.js

a105700

grgr-dkrk requested a review from kacieb May 11, 2021 00:05

Merge branch 'master' into fix/android_importantForAccessibility

7eeafc7

react-native-bot added the Bug label Oct 1, 2021

react-native-bot added the Platform: Android Android applications. label Oct 1, 2021

grgr-dkrk added 2 commits October 3, 2021 17:34

Merge branch 'main' into fix/android_importantForAccessibility

c61ea55

chore: update snapshot of Button-test.js

b8747ee

grgr-dkrk requested a review from lunaleaps October 27, 2021 12:56

fabOnReact mentioned this pull request Apr 5, 2022

Android: Role description is announced before the components text, rather than after #31042 fabOnReact/react-native-notes#14

Closed

fabOnReact mentioned this pull request Jul 22, 2022

adding importantForAccessibility for Text, Button, ImageBackground #34245

Closed

fabOnReact closed this Jan 11, 2023

fabOnReact reopened this Jan 11, 2023

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 21, 2023

github-actions bot closed this Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for importantForAccessibility for some components #31296

Added support for importantForAccessibility for some components #31296

grgr-dkrk commented Apr 4, 2021 •

edited

Loading

analysis-bot commented Apr 4, 2021 •

edited

Loading

kacieb Apr 16, 2021

grgr-dkrk Apr 18, 2021

blavalla Apr 19, 2021 •

edited

Loading

kacieb commented Apr 16, 2021

blavalla left a comment

blavalla Apr 19, 2021

kacieb Apr 21, 2021

lunaleaps Oct 4, 2021

blavalla Apr 23, 2021

grgr-dkrk Apr 24, 2021

lunaleaps commented May 3, 2021

grgr-dkrk commented May 4, 2021

kacieb left a comment

kacieb May 6, 2021

grgr-dkrk May 10, 2021

analysis-bot commented Jun 25, 2021 •

edited

Loading

fabOnReact commented Dec 20, 2021 •

edited

Loading

fabOnReact commented Jul 22, 2022

fabOnReact commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023

github-actions bot commented Dec 21, 2023

github-actions bot commented Dec 29, 2023

Added support for importantForAccessibility for some components #31296

Added support for importantForAccessibility for some components #31296

Conversation

grgr-dkrk commented Apr 4, 2021 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Apr 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blavalla Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

kacieb commented Apr 16, 2021

blavalla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunaleaps commented May 3, 2021

grgr-dkrk commented May 4, 2021

kacieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot commented Jun 25, 2021 • edited Loading

fabOnReact commented Dec 20, 2021 • edited Loading

fabOnReact commented Jul 22, 2022

fabOnReact commented Jan 11, 2023 • edited Loading

github-actions bot commented Jan 11, 2023

github-actions bot commented Dec 21, 2023

github-actions bot commented Dec 29, 2023

grgr-dkrk commented Apr 4, 2021 •

edited

Loading

analysis-bot commented Apr 4, 2021 •

edited

Loading

blavalla Apr 19, 2021 •

edited

Loading

analysis-bot commented Jun 25, 2021 •

edited

Loading

fabOnReact commented Dec 20, 2021 •

edited

Loading

fabOnReact commented Jan 11, 2023 •

edited

Loading